-
Notifications
You must be signed in to change notification settings - Fork 24.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Run more search Rest tests in a CCS setup #86521
Conversation
Pinging @elastic/clients-team (Team:Clients) |
52b1fe9
to
5c1eb3a
Compare
Pinging @elastic/es-search (Team:Search) |
5c1eb3a
to
8f862eb
Compare
Currently we only test a small subset of cross cluster search in rest tests in the 'multi-cluster-search' qa module. In order to increase test coverage for basic CCS setups, this change adds a new qa modula that uses a subset of existing search rest tests tests and runs them in a CCS scenario. Relates to elastic#84481
8f862eb
to
e7e1b77
Compare
94ec9eb
to
2ca0205
Compare
Some of the ccs compatible endpoints (like e.g. search_template) have their yaml rest tests under "modules" or x-pack (terms_enum), I don't yet if/how we can run them from here, so this should probably be a follow up task. |
I added the "indices.resolve_index" api test to the covered subset of rest tests, also fixed a small issue that led to some test assertions being ignored where they shouldn't have, which led to a few overlooked places in "field_caps" where the testrunner still needed to rewrite the expected matches. Locally 751 tests are passing now, with only the blacklisted ones skipped atm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple of questions, this looks right to me though, thanks for working on this!
@javanna I pushed an update adressing your comments. I can also add the "_search_shard" endpoint to the APIs we route to the local search cluster instead of the remote holding the data if that API is supposed to support CCS and we want to include it in the testing here. |
@cbuescher I do not think that the search shards API supports cross-cluster calls, does it? |
No, I also think it doesn't. So nothing to do there then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
qa/ccs-common-rest/build.gradle
Outdated
|
||
systemProperty 'tests.rest.blacklist', | ||
[ | ||
// TODO look into fixing these |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like the reasons why these are in the blacklist are good, so maybe the TODO can be removed?
@javanna thanks for the heads up. I think I can include "_terms_enum" and "async_search" yaml tests from xpack before merging, might need another few commits on this PR though. |
qa/ccs-common-rest/src/test/java/org/elasticsearch/test/rest/yaml/CcsCommonYamlTestSuiteIT.java
Show resolved
Hide resolved
@javanna just fyi I added the "asych_search" tests from x-pack to be included in the suites we are running here. Adding "terms_enum" in a similar way proved to be difficult since the basic tests under "x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/terms_enum/10_basic.yml" require a security setup which we don't have here. |
Currently we only test a small subset of cross cluster search in rest tests in the 'multi-cluster-search' qa module. In order to increase test coverage for basic CCS setups, this change adds a new qa modula that uses a subset of existing search rest tests tests and runs them in a CCS scenario.
Relates to #84481